Conversation
c836aff to
399071b
Compare
Add comprehensive FreeBSD support to brpc, building on the existing partial FreeBSD code already present in the codebase. CMakeLists.txt: - Add NO_PTHREAD_MUTEX_HOOK on FreeBSD to prevent static init deadlock caused by bthread's pthread_mutex_lock interposition conflicting with libc++ ios_base::Init during early process initialization - Add -lexecinfo for backtrace support - Add FreeBSD-specific source files (platform_thread_freebsd.cc, sys_string_conversions_posix.cc) Source changes (all guarded by OS_FREEBSD, no impact on Linux/macOS): - butil/compat.h: Add FreeBSD kqueue path and pthread_getthreadid_np() - butil/process_util.cc: FreeBSD ReadCommandLine via ps (like macOS) - butil/fd_utility.cpp: kqueue fd handling - butil/logging.cc: Rename FLAGS_v/vmodule/minloglevel to avoid gflags duplicate registration with system glog on FreeBSD - brpc/socket.cpp: FreeBSD TCP_INFO support - bthread/: FreeBSD futex emulation, context switching, fd handling - bvar/default_variables.cpp: FreeBSD /proc fallback Most changes piggyback on existing macOS/Darwin code paths since both use kqueue. The NO_PTHREAD_MUTEX_HOOK fix is critical — without it, any FreeBSD application linking brpc will deadlock during startup.
399071b to
2a91e67
Compare
|
LGTM |
There was a problem hiding this comment.
Pull request overview
This PR aims to complete FreeBSD support across butil, bthread, and brpc, primarily by routing FreeBSD to existing kqueue-based code paths (similar to macOS) and adding FreeBSD-specific implementations/guards in a few platform-dependent areas.
Changes:
- Add FreeBSD build/link and source wiring in CMake (including
NO_PTHREAD_MUTEX_HOOKand FreeBSD-only sources). - Extend multiple POSIX/macOS code paths to include FreeBSD (kqueue usage, errno handling, socket options, etc.).
- Add/adjust FreeBSD-specific implementations for process path discovery, TCP_INFO, and some bthread internals.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Adds FreeBSD-specific compiler defines, libraries, and wires FreeBSD sources into BUTIL build. |
src/bvar/default_variables.cpp |
Adds FreeBSD fallback behavior for proc I/O stats collection. |
src/butil/process_util.cc |
Adds FreeBSD implementation for command line reading and absolute path lookup. |
src/butil/popen.cpp |
Adds FreeBSD include needed for wait/waitpid usage. |
src/butil/logging.cc |
Renames certain gflags on FreeBSD to avoid collisions with system glog. |
src/butil/fd_utility.cpp |
Adds FreeBSD TCP_INFO path for is_connected(). |
src/butil/errno.cpp |
Treats FreeBSD like macOS for strerror_r variant handling. |
src/butil/endpoint.cpp |
Extends kqueue-related paths to FreeBSD. |
src/butil/details/extended_endpoint.hpp |
Sets BSD sockaddr length fields on FreeBSD. |
src/butil/compat.h |
Adds FreeBSD kqueue header and FreeBSD pthread numeric-id behavior. |
src/bthread/task_group.cpp |
Uses FreeBSD-compatible thread id formatting (shares macOS path). |
src/bthread/task_control.cpp |
Adds FreeBSD CPU affinity binding implementation. |
src/bthread/sys_futex.h |
Routes FreeBSD to the macOS-style futex emulation interface. |
src/bthread/sys_futex.cpp |
Routes FreeBSD to the macOS-style futex emulation implementation. |
src/bthread/mutex.cpp |
Adds FreeBSD dlsym setup for pthread mutex function pointers. |
src/bthread/fd.cpp |
Extends kqueue-based fd wait/connect implementation to FreeBSD. |
src/bthread/errno.cpp |
Routes FreeBSD to the macOS-style errno accessor. |
src/bthread/context.h |
Maps FreeBSD arch selection to existing linux fcontext backends. |
src/brpc/socket.cpp |
Adds FreeBSD keepalive/TCP_INFO support and kqueue-related waits. |
src/brpc/policy/domain_naming_service.cpp |
Extends macOS DNS resolution path to FreeBSD. |
src/brpc/event_dispatcher.cpp |
Routes FreeBSD to the kqueue event dispatcher implementation. |
src/brpc/controller.cpp |
Uses sig_t handler type for FreeBSD (shares macOS path). |
Comments suppressed due to low confidence (1)
src/brpc/policy/domain_naming_service.cpp:115
- Treating FreeBSD like macOS here switches to
gethostbyname(), but on FreeBSDgethostbyname()is not guaranteed to be thread-safe (unlike the macOS TLS behavior described in the comment). The existinggethostbyname_r()path below should work on FreeBSD as well and avoids potential data races/corruption under concurrent DNS lookups. Consider removing|| defined(OS_FREEBSD)from this#ifand keeping FreeBSD on the re-entrantgethostbyname_r()implementation.
#if defined(OS_MACOSX) || defined(OS_FREEBSD)
_aux_buf_len = 0; // suppress unused warning
// gethostbyname on MAC is thread-safe (with current usage) since the
// returned hostent is TLS. Check following link for the ref:
// https://lists.apple.com/archives/darwin-dev/2006/May/msg00008.html
struct hostent* result = gethostbyname(buf);
if (result == NULL) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #if defined(OS_FREEBSD) | ||
| // Rename flags to avoid gflags duplicate registration with system glog. | ||
| // glog defines "v" and "vmodule"; we define "brpc_verbose" and "brpc_vmodule" | ||
| // and create local aliases so brpc code can still use FLAGS_v / FLAGS_vmodule. | ||
| DEFINE_int32(brpc_verbose, 0, "Show all VLOG(m) messages for m <= this." | ||
| " Overridable by --brpc_vmodule."); | ||
| DEFINE_string(brpc_vmodule, "", "per-module verbose level."); | ||
| #define FLAGS_v FLAGS_brpc_verbose | ||
| #define FLAGS_vmodule FLAGS_brpc_vmodule | ||
| #else | ||
| DEFINE_int32(v, 0, "Show all VLOG(m) messages for m <= this." | ||
| " Overridable by --vmodule."); | ||
| DEFINE_string(vmodule, "", "per-module verbose level." | ||
| " Argument is a comma-separated list of MODULE_NAME=LOG_LEVEL." | ||
| " MODULE_NAME is a glob pattern, matched against the filename base" | ||
| " (that is, name ignoring .cpp/.h)." | ||
| " LOG_LEVEL overrides any value given by --v."); | ||
| #endif | ||
|
|
||
| DEFINE_bool(log_pid, false, "Log process id"); | ||
|
|
||
| DEFINE_bool(log_bid, true, "Log bthread id"); | ||
|
|
||
| #if defined(OS_FREEBSD) | ||
| DEFINE_int32(brpc_minloglevel, 0, "Any log at or above this level will be " | ||
| "displayed. Anything below this level will be silently ignored. " | ||
| "0=INFO 1=NOTICE 2=WARNING 3=ERROR 4=FATAL"); | ||
| BUTIL_VALIDATE_GFLAG(brpc_minloglevel, butil::NonNegativeInteger); | ||
| #define FLAGS_minloglevel FLAGS_brpc_minloglevel | ||
| #else | ||
| DEFINE_int32(minloglevel, 0, "Any log at or above this level will be " | ||
| "displayed. Anything below this level will be silently ignored. " | ||
| "0=INFO 1=NOTICE 2=WARNING 3=ERROR 4=FATAL"); | ||
| BUTIL_VALIDATE_GFLAG(minloglevel, butil::NonNegativeInteger); | ||
| #endif |
There was a problem hiding this comment.
On FreeBSD this file renames the gflags from v/vmodule/minloglevel to brpc_* and only adds #define FLAGS_v ... aliases in this .cc file. However src/butil/logging.h still has DECLARE_int32(v); and uses ::logging::FLAGS_v (and the code registers validators against FLAGS_vmodule), so other translation units will still expect the logging::FLAGS_v/FLAGS_vmodule/FLAGS_minloglevel symbols to exist and will fail to link on FreeBSD. The aliases need to be made visible to all users (e.g., add corresponding #if defined(OS_FREEBSD) mapping and DECLARE_* updates in logging.h, and adjust any flag-name based code/tests as needed).
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DNO_PTHREAD_MUTEX_HOOK") | ||
| set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DNO_PTHREAD_MUTEX_HOOK") |
There was a problem hiding this comment.
The FreeBSD -DNO_PTHREAD_MUTEX_HOOK addition to CMAKE_C_FLAGS is effectively lost because CMAKE_C_FLAGS is re-assigned unconditionally a few lines later (line 155), overwriting the earlier append (line 153). If this define is intended for all C/C++ compilation, add it to CMAKE_CPP_FLAGS (so both C and CXX inherit it) or move the FreeBSD block after the final set(CMAKE_C_FLAGS ...) assignment.
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DNO_PTHREAD_MUTEX_HOOK") | |
| set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DNO_PTHREAD_MUTEX_HOOK") | |
| set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} -DNO_PTHREAD_MUTEX_HOOK") |
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
brpc has partial FreeBSD support (27 source files already contain
OS_FREEBSD/__FreeBSD__references), but it does not compile or run on FreeBSD. This PR completes that support.The most critical issue is a static initialization deadlock: bthread interposes
pthread_mutex_lockfor contention profiling. On FreeBSD, during early process init,libc++.so'sios_base::Init::Init()callspthread_mutex_lockto initializestd::locale::classic(). bthread's interposed version callspthread_once(init_sys_mutex_lock), which itself needspthread_mutex_lock— causing infinite recursion. Without theNO_PTHREAD_MUTEX_HOOKfix in this PR, any FreeBSD application linking brpc will deadlock at startup.This is PR 2 of 3 for FreeBSD support:
What is changed and the side effects?
Changed:
All source changes are guarded by
#if defined(OS_FREEBSD)— zero impact on Linux or macOS builds. Most changes piggyback on existing macOS/Darwin code paths since both use kqueue.CMakeLists.txt:
NO_PTHREAD_MUTEX_HOOKon FreeBSD to prevent static init deadlock caused by bthread'spthread_mutex_lockinterposition conflicting withlibc++ios_base::Initduring early process initialization-lexecinfofor backtrace supportplatform_thread_freebsd.cc,sys_string_conversions_posix.cc— already present in the tree but not compiled)Source changes:
butil/compat.h: Add FreeBSD kqueue path andpthread_getthreadid_np()butil/process_util.cc: FreeBSDReadCommandLineviaps(like macOS)butil/fd_utility.cpp: kqueue fd handlingbutil/logging.cc: RenameFLAGS_v/vmodule/minloglevelto avoid gflags duplicate registration with system glog on FreeBSDbrpc/socket.cpp: FreeBSDTCP_INFOsupportbthread/: FreeBSD futex emulation, context switching, fd handlingbvar/default_variables.cpp: FreeBSD/procfallbackSide effects:
Performance effects:
None — all changes are behind
OS_FREEBSDguards and do not affect existing platforms.Breaking backward compatibility:
None — this is additive FreeBSD support only.
Check List: